Skip to content

Conversation

@AlessandroMiola
Copy link
Contributor

@AlessandroMiola AlessandroMiola commented Oct 11, 2024

What type of PR is this? (check all applicable)

  • ๐Ÿ’พ Refactor
  • โœจ Feature
  • ๐Ÿ› Bug Fix
  • ๐Ÿ”ง Optimization
  • ๐Ÿ“ Documentation
  • โœ… Test
  • ๐Ÿณ Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below.

Attempt at adding downstream tests for marimo to the CI pipeline.

Notes:

  • marimo's full test suite also includes additional tests for frontend/cli etc.; I've omitted them as not relevant to the python backend testing functionality.
  • marimo leverages on hatch, though pyproject.toml shows the following

    [tool.hatch]
    installer = "uv"

  • also, test:test, test-optional:test and typecheck:check do refer to the following pytest and mypy commands

    [tool.hatch.envs.test.scripts]
    test = "pytest{env:HATCH_TEST_ARGS:} {args:tests}"
    ...
    [tool.hatch.envs.test-optional]
    template = "test"
    ...
    [tool.hatch.envs.typecheck.scripts]
    check = "mypy marimo/"

All this said, I've kept the commands as-is (trying to align with the approach in #1161, even though I haven't referenced make commands as they do hardcode the python version in within).

Please let me know your thoughts on these points so that I can tweak accordingly :) (assuming that everything works as intended ๐Ÿฅถ)

@MarcoGorelli
Copy link
Member

thanks for working on this! @akshayka fancy taking a look? reckon this would be good enough to cover how you're using narwhals in marimo?

Copy link
Contributor

@akshayka akshayka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! I left a small comment that might help make CI pass (and also make it run faster).

Thank you for doing this!

- name: install-marimo-dev
run: |
cd marimo
uv pip install -e ".[dev]" --system
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do these need to be editable installs? not sure if there is a perf implication

@AlessandroMiola AlessandroMiola marked this pull request as ready for review October 14, 2024 21:22
Copy link
Contributor

@akshayka akshayka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @AlessandroMiola , and @akshayka + @mscolnick for helping out!

the optional test run is a bit long compared with other runs (6 mins, vs usually 2-3 mins), but for now I think it's worth to include it (maybe later if we have too many tests we can only run some on certain tags, but for now i'd say let's play it safe)

@MarcoGorelli MarcoGorelli merged commit 3f186da into narwhals-dev:main Oct 15, 2024
27 checks passed
@AlessandroMiola AlessandroMiola deleted the ci-marimo-downstream-tests branch October 15, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: add Marimo to downstream tests

4 participants